Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Bunch methods to allow live updates and loading from json #170

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

collijk
Copy link

@collijk collijk commented Apr 28, 2024

This is a request for feedback to address #12 and #14 as it technically has some breaking api changes and I'm unsure what kinds of guarantees you have with downstream users.

New Features

  • Bunch(**correctly_formatted_provider_dict) now makes a providers object from whatever's in the dict.
  • providers[new_provider_name] = new_provider now works whenever new_provider_name is a str and new_provider is a Bunch, a TileProvider or a correctly formatted dict representing a Bunch or TileProvider
  • providers.update(**{provider_name: new_provider, ...}) Now works wherever provider_name and new_provider match the __setitem__ criteria.
  • Bunch.from_json(json_string) now does the work of the _load_json function. We could reasonably make this also load json files or ditch the method entirely and have providers.py make the json.loads call.

Breaking changes

  • __init__, __setitem__ and update no longer function like their dict counterparts for the Bunch class. I'd be suprised if anyone depended on this functionality though.
  • isinstance(TileProvider, Bunch) is no longer true, and TileProvider no longer has access to Bunch methods, consequently. Instead, TileProvider directly inherits from dict and implements it's own copy of __getattr__. I think this is technically more in line with what you want, but may have consequences I can't see.

Other thoughts

If you can provide more guidance about downstream use cases, I think it would be worth making the following adjustments:

  • Changing the base type of Bunch from dict to collections.UserDict[str, Bunch | TileProvider] and adding in appropriate runtime validation to ensure it stays consistent
  • Changing the base type of TileProvider to either a pydantic.BaseModel or a UserDict[str, str | int] and providing validation for required and optional fields.

I think both changes would provide better usability (ie would fail fast and noisily) if downstream users are adding their own sources and formatting them poorly.

@martinfleis
Copy link
Member

All fine with me. I don't expect people relying on anything like this in downstream.

Changing the base type of TileProvider to either a pydantic.BaseModel or a UserDict[str, str | int] and providing validation for required and optional fields.

We have intentionally no dependencies, so depending on pydantic is no-go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants